-
Notifications
You must be signed in to change notification settings - Fork 73
feat: add ExpireSnapshots as concrete class #370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
bfd5280 to
e1f179b
Compare
src/iceberg/test/CMakeLists.txt
Outdated
| literal_test.cc | ||
| predicate_test.cc) | ||
|
|
||
| add_iceberg_test(update_test SOURCES ../update/test/expire_snapshots_test.cc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think this test file should be placed under src/iceberg/test/update/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
e1f179b to
36755e3
Compare
Add ExpireSnapshots as a concrete implementation in iceberg/update/
following the PendingUpdate pattern.
Features:
- Fluent API builder methods: ExpireSnapshotId, ExpireOlderThan,
RetainLast, DeleteWith, SetCleanupLevel
- Extends PendingUpdateTyped<vector<shared_ptr<Snapshot>>>
- Takes Table* in constructor for access to table metadata
- Placeholder implementations for Apply() and Commit()
Directory structure:
- src/iceberg/update/expire_snapshots.{h,cc}
- src/iceberg/update/CMakeLists.txt
- src/iceberg/update/meson.build
- Updated build files (CMakeLists.txt, meson.build)
Add comprehensive test suite for ExpireSnapshots API. Tests cover: - Fluent API chaining - ExpireSnapshotId single and multiple - ExpireOlderThan timestamp-based expiration - RetainLast to keep recent snapshots - DeleteWith custom deletion callback - SetCleanupLevel with different levels (None, MetadataOnly, All) - Combined configuration scenarios Test file location: src/iceberg/test/update/expire_snapshots_test.cc
36755e3 to
66809ae
Compare
src/iceberg/table.h
Outdated
| /// \brief Create a new expire snapshots operation for this table | ||
| /// | ||
| /// \return a shared pointer to the new ExpireSnapshots operation | ||
| virtual std::shared_ptr<ExpireSnapshots> NewExpireSnapshots(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: return unique_ptr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed earlier but didn't reply this comment. Sorry.
Change Table::NewExpireSnapshots() and Transaction::NewExpireSnapshots() to return std::unique_ptr instead of std::shared_ptr for consistency with other factory methods (NewScan, NewTransaction) that create new objects with exclusive ownership transfer to the caller.
66cffc5 to
2cb6080
Compare
Add PendingUpdateTyped template class to provide type-safe ApplyTyped() method for table metadata changes. This allows different operations to return specific result types (e.g., vector<Snapshot> for ExpireSnapshots). Changes: - Add PendingUpdateTyped<T> template extending PendingUpdate - Add ApplyTyped() method returning Result<T> for typed operations - Keep Apply() returning Status in base PendingUpdate for compatibility - PendingUpdateTyped provides default Apply() implementation that calls ApplyTyped() - Update ExpireSnapshots to use ApplyTyped() instead of Apply() This design allows: - ExpireSnapshots to return Result<vector<Snapshot>> from ApplyTyped() - UpdateProperties to continue using Status Apply() from base - All PendingUpdate subclasses to work polymorphically through base interface
2cb6080 to
79edadb
Compare
Add ExpireSnapshots as a concrete implementation extending PendingUpdateTyped, following Gang's suggestion to avoid pure interfaces when there's only one implementation.
Changes:
This design provides:
Full implementation of Apply() and Commit() will be added in follow-up PRs.